Skip to content

Add unsafe test to check that dropped values are zeroized#1180

Closed
dvdplm wants to merge 2 commits into
RustCrypto:masterfrom
dvdplm:dp-test-zeroization
Closed

Add unsafe test to check that dropped values are zeroized#1180
dvdplm wants to merge 2 commits into
RustCrypto:masterfrom
dvdplm:dp-test-zeroization

Conversation

@dvdplm

@dvdplm dvdplm commented May 5, 2025

Copy link
Copy Markdown

Test that zeroization actually occurs when a boxed value goes out of scope.

Uses unsafe code, likely relies on UB and adds a custom allocator, hence the draft: is something like this acceptable?

c.f. iqlusioninc/crates#1310

Comment thread zeroize/tests/zeroize.rs Outdated
Comment thread zeroize/Cargo.toml Outdated
Comment thread zeroize/tests/zeroize.rs Outdated
@dvdplm

dvdplm commented May 7, 2025

Copy link
Copy Markdown
Author

I guess the original question still stands: is something like this acceptable? I'm on the fence myself, hence the draft.

@newpavlov

Copy link
Copy Markdown
Member

Personally, while I am fine with having such test, I don't see much worth in it. I will leave the final decision to @tarcieri.

As a small suggestion, it may be worth to slightly modify how the test is implemented. Instead of reading leaked memory in the test itself, it may be better to allocate a known size (e.g. 1024 bytes) using SecretBox and then in dealloc check that deallocated memory with such size should be filled with zeros.

@dvdplm dvdplm marked this pull request as ready for review May 16, 2025 14:20
@newpavlov

Copy link
Copy Markdown
Member

@tarcieri WDYT? I lean towards closing this PR.

@tarcieri

tarcieri commented Aug 6, 2025

Copy link
Copy Markdown
Member

While it would be nice to have a test, I worry about including one that has UB

@newpavlov

Copy link
Copy Markdown
Member

Closing in favor of #1199

@newpavlov newpavlov closed this Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants